Size Change: +1.08 kB (0%)

Total Size: 1.12 MB

_{compressed-size-action}

cc @noisysocks

I am thinking we need to implement something that demonstrates the value of this refactor in order to gauge whether the extra bit of complexity is worth it or not. Could you maybe add a rough pass at undo/redo to this PR to see how it looks? That's what I imagine the primary value of using @wordpress/data is.

Fixing #22625 could be a good first implementation of an edit-navigation store as well—moving the selected menu id from state to store would solve that.

The functions in wp.data.select( '...' ) and wp.data.dispatch( '...' ) are callable by third parties and I generally like to think of them as official APIs for all WordPress developers to use.

With that in mind, it's weird to me that this PR has wp.data.select( 'core/edit-navigation' ).getMenuItemIdToClientIdMapping(), wp.data.dispatch( 'core/edit-navigation' ).setMenuItemsToClientIdMapping(), and wp.data.dispatch( 'core/edit-navigation' ).assignMenuItemIdToClientId(). These functions are awfully specific. They expose the concept of mapping menu item IDs to block client IDs which is an implementation detail that I don't think will be useful outside of our needs.

Could we move the code that's in useNavigationBlockEditor into selectors and actions that define a concrete API for working with block-based menus? How these selectors and actions convert blocks to and from menu items would then be an implementation detail. I don't think (but am not 100%!) that there's anything in useNavigationBlockEditor that can't be done using actions which use select() and apiFetch() controls.

To illustrate, here's what I imagine you'd be able to do in the DevTools console:

// Hits GET /wp/v2/menu-items, munges the data into Link blocks, and returns
// everything as a fake post.
const post = wp.data.select( 'edit-navigation' ).getNavigationPost( /* menuId: */ 123 ); 
console.log( post.id ); // navigation-post-123
console.log( post.blocks ); // [ { name: 'core/navigation', ... } ]

// Hits POST /wp/v2/menu-items once for every Link block that doesn't have an
// associated menu item. (IDK what a good name for this is.)
wp.data.dispatch( 'edit-navigation' ).createMissingMenuItems( post );

// Hits the Customiser endpoint and saves everything.
wp.data.dispatch( 'edit-navigation' ).saveNavigationPost( post );

// If we put our fake post into the 'core' registry then we can do things like
// undo and redo.
wp.data.dispatch( 'core' ).receiveEntityRecords( 'postType', 'post', [ post ] );
wp.data.dispatch( 'core' ).editEntityRecord(
	'postType',
	'post',
	'navigation-post-123',
	{ blocks: [
		...post.blocks,
		{ name: 'core/paragraph', content: 'This will be undone!', ... }
	] }
);
wp.data.dispatch( 'core' ).undo();
wp.data.dispatch( 'core' ).redo();

Does that make sense? Thoughts?

The functions in wp.data.select( '...' ) and wp.data.dispatch( '...' ) are callable by third parties and I generally like to think of them as official APIs for all WordPress developers to use.

TIL - that's a good mental model, thank you! I think exposing wp.data.dispatch( 'edit-navigation' ).saveNavigationPost( post ); is a brilliant idea - let's explore that more.

They expose the concept of mapping menu item IDs to block client IDs which is an implementation detail that I don't think will be useful outside of our needs.

@noisysocks while I agree the name getMenuItemIdToClientIdMapping() is awful, we need to store that information somewhere though. At the moment we use a ref which works fine but doesn't really allow us to expose an API like wp.data.select( 'core/edit-navigation' ).getMenuItem( clientId ) which is what you mentioned in the following comment: #22022 (comment)

My understanding is that in order to expose a getMenuItem() selector, the clientId<->menuId mapping has to be stored in redux state. This in turn means that we need a way to perform reads and writes.

An alternative approach would be storing menuItemId in block attributes - this should remove the need for any mapping-related actions and selectors while still making it possible to expose a getMenuItem( clientId ) selector - what do you think?

@adamziel I think you're on the right path storing this in state, but a lot the comments were probably about making the public facing API friendlier rather than using an alternate solution like block attributes.

The block editor store has a lot of relevance in that there are reducers that encapsulate things like mappings (block order, parent/child relationships), but they're not necessarily exposed to the user directly through actions. For example insertBlock will update the order, and parents. Similarly a lot of the commonly used selectors are a bit more higher-level than the reducers they read from, though with selectors there does tend to be a lot more composition.

It might be that we have an updateMenuItem action. Separately a reducer for storing the mapping between blocks and menu items might have a case for that action (naming is less important as the reducer isn't public). getMenuItemIdForBlock or getClientIdForMenuItem also sound like reasonable selectors.

In terms of saveNavigationPost, not sure if we'd want to go for something like a generator action like the editor store has for savePost:
https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/store/actions.js#L231

That's another way to encapsulate the various working parts, but lets have more conversation on that.

Fixing #22625 could be a good first implementation of an edit-navigation store as well—moving the selected menu id from state to store would solve that.

@talldan @adamziel #22428 fixes that, because when we delete an entity we don't need to use state anymore. I think @noisysocks 's suggestion with demo-ing undo/redo in the navigation screen is a better demonstration of this PR's powers!

@noisysocks I explored getting rid of most hooks in favor of the actions, reducers, and such. Interestingly enough it ended up being quite similar to the API you proposed. I played with undo/redo and it seems to work 🎉

Even better - there is no more weirdness with passing select to nested callbacks in react hooks.

We also now have a selector called getMenuItemForClientId() which does exactly what the name says. Implementing an inverse selector should be pretty easy too.

@talldan I also ended up moving the mapping to post.meta - if we treat the post as main model for the editor then it makes sense to keep that data there.

There are a few minor bugs that I noticed, but let's have more discussion about the approach before I go ahead and start fixing them.

@draganescu You might be thinking of a different issue. #22625 is related to the menuId, setMenuId state. It doesn't look like #22428 touches that code.

@talldan I also ended up moving the mapping to post.meta - if we treat the post as main model for the editor then it makes sense to keep that data there.

That's interesting! What are the pros and cons for doing this versus storing the mapping in the edit-navigation store?

I addressed all your feedback @noisysocks, this is ready for another round. Unfortunately it still attempts to request the post from the API - I will debug it tomorrow.

Semi-relatedly: is there anything we can do to make it clearer that a navigation post is not a "real" post that can be persisted and accessed via /wp/v2/posts?

Probably not much other than naming. UnsavableNavigationPost? StubNavigationPost? InMemoryNavigationPost?